-
Notifications
You must be signed in to change notification settings - Fork 34
Return verification #1490
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Return verification #1490
Conversation
🦋 Changeset detectedLatest commit: b27a265 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
91d1ae5 to
2d7da9b
Compare
|
LGTM |
|
friendly ping @bdehamer |
bdehamer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@indexzero Thanks for contributing this change!
Everything looks great. My only request is that you also run npx changeset and commit the resulting changeset file (minor version bump to the sigstore package).
|
@bdehamer roger that! I had put this down for the few weeks around JSConf, but will button it up this week for a final review & merge 🫶 |
58b5d50 to
823e9a5
Compare
|
@bdehamer the saying "Open source happens when you're busy making other plans" never ceases to ring true for me 😅 I appreciate your continued patience 🙏 PR should be buttoned up to merge:
|
`verify(bundle[, payload][, options])` now returns a `Signer` object containing the public key and identity information from the verification. This updates the behavior of `verify` to be consistent with the `sigstore-go` implementation where the `result` is returned to but not consumed by `cosign`. As such the corresponding `packages/cli` functionality was not updated to maximize implementation cohesion Note: technically **NOT** breaking as it is a patch to match the existing `sigstore-go` implementation Fixes: sigstore#1489 Citations: - [Existing `verify` behavior](https://github.com/sigstore/sigstore-go/blob/201a35a/pkg/verify/signed_entity.go#L797) - [Existing `cosign verify-blob` behavior](https://github.com/sigstore/cosign/blob/8e3a787/cmd/cosign/cli/verify/verify_blob.go#L192) --------- Co-authored-by: Claude <[email protected]> Signed-off-by: indexzero <[email protected]>
Signed-off-by: Brian DeHamer <[email protected]>
823e9a5 to
b27a265
Compare
Howdy all 👋 looking forward to contributing to the project. Many thanks to @wlynch and @hectorj2f for helping me ramp up on the codebases 🫶
Summary
This PR updates the behavior of
verifyto be consistent with thesigstore-goimplementation. Thisresultis returned to but not consumed bycosignso the correspondingpackages/clifunctionality was not updated as part of this PR to maximize cohesionverifybehavior: sigstore/sigstore-go/pkg/verify/signed_entity.go#798cosign verify-blobbehavior: sigstore/cosign/cmd/cosign/cli/verify/verify_blob.go#192Release Note
verify(bundle[, payload][, options])now returns aSignerobject containing the public key and identity information from the verification.Documentation
Updated
README.mdto indicate the new returnAdditional Remarks
🤖
claudehelped here, but all of thepackages/*code itself was 100% old fashioned fingers on keyboard. I don't usejestoften so it was helpful in adding the test coverage for this change. No offense will be taken if it is updated or removed.